[wip] feat: add support for storage policies#697
Conversation
|
CI is failing. Be sure to run "make generate" and push to your fork. Also, please sign your commits per the contributing guidelines. Thanks! |
a5ad88b to
a1eb4e3
Compare
|
Hey, |
a1eb4e3 to
fa65dfc
Compare
|
Hi @pascalinthecloud 👋🏻 - I'm hoping to be able to review and test e2e later this week. Ryan |
ac915b9 to
081aa3b
Compare
… and VM creation Signed-off-by: Pascal T. <pascal@toepke.dev>
Signed-off-by: Pascal T. <pascal@toepke.dev>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Pascal T. <pascal@toepke.dev>
081aa3b to
84241e7
Compare
| // Defaults to the first controller, `(0)`. | ||
| DiskControllerIndex int `mapstructure:"disk_controller_index"` | ||
| // The name of the storage policy to apply to the disk. The storage policy | ||
| // must already exist on the vCenter Server. If not specified, the default |
There was a problem hiding this comment.
Nit: Updated product terminology.
| // must already exist on the vCenter Server. If not specified, the default | |
| // must already exist on the vCenter instance. If not specified, the default |
There was a problem hiding this comment.
Update and then run make generate before committing.
tenthirtyam
left a comment
There was a problem hiding this comment.
Hi @pascalinthecloud! 👋🏻 Thanks for the contribution, the disk-level VirtualMachineDefinedProfileSpec wiring and the VmProfile fallback for the VM home look correct.
I noticed one behavioral gap during my testing that I'd like to see addressed before this moved forward for merge:
storage_policy currently affects only the device/VM profile spec, but datastore placement still falls through to the existing logic in LocationConfig.Prepare and VCenterDriver.FindDatastore. That means if you provide a storage_policy on a cluster (or a host with more than one datastore) and omit both datastore and datastore_cluster you will still hit:
host has multiple datastores; specify the datastore nameWe should ensure the when you supply a policy that PBM selects a compliant datastore.
Could you extend the PR so that when storage_policy is set and neither datastore nor datastore_cluster is specified, that PBM will resolve a compliant placement and uses the result as the effective datastore?
In the meantime, I'm going to make as draft and [wip].
Summary
Adds per-disk storage policy support to the
vsphere-isobuilder (and the sharedDiskConfigused byvsphere-clone).A new optional
storage_policyfield on eachstorage {}block accepts a vSphere storage policy name. At build time the plugin resolves the name to a profile UUID via the Policy-Based Management (PBM) API and attaches aVirtualMachineDefinedProfileSpecto the disk's device config spec. The VM home files (.vmx,.nvram, etc.) are also placed under the same policy viaVirtualMachineConfigSpec.VmProfile, which the vSphere API requires whenever any disk carries a per-disk profile. If the field is left empty the behaviour is unchanged — the datastore's default policy applies.The PBM client is initialised lazily on first use from the existing VIM25 session, so no new connection or credential is needed.
Example usage:
Type
fix: Bug Fixfeat: Feature or Enhancementdocs: Documentationrefactor: Refactoringchore: Build, Dependencies, Workflows, etc.other: Other (Please describe.)Breaking Changes?
The new field is optional. Existing templates without
storage_policyare unaffected.Tests
New unit tests:
driver/disk_test.goTestAddStorageDevices_WithStoragePolicyVirtualMachineDefinedProfileSpecis attached to the correct disk'sVirtualDeviceConfigSpec; absent on disks without a policydriver/vm_test.goTestVirtualMachineDriver_CreateVM_WithStoragePolicyCreateVMsucceeds end-to-end via the govmomi simulator when a disk carries aStoragePolicyID, exercising theVmProfilebranch of the VM config speciso/step_create_test.goTestStepCreateVM_Run_WithStoragePolicyStoragePolicyIDon the driver diskiso/step_create_test.goTestStepCreateVM_Run_StoragePolicyNotFoundOutput:
The feature was also validated against a live vCenter with a real storage policy.
Documentation
docs-partials/builder/vsphere/common/DiskConfig-not-required.mdxwas updated with the newstorage_policyfield. This partial is shared betweenvsphere-isoandvsphere-clonedocs.Issue References
Closes #18
Release Note
Additional Information
Implementation notes:
govmomi/pbm) is already an indirect dependency viagovmomi— no new module dependency is introduced.Runstep), not duringPrepare, so the error message includes the vCenter-side policy name and is actionable.VirtualMachineConfigSpec.VmProfileis set to the first disk's policy when any disk carries a policy. This is required by the vSphere API and mirrors the behaviour of other tools (Terraform provider, govc). If disks have mixed policies the VM home uses the first disk's policy; a dedicated VM-home policy field can be added in a follow-up.